Simplify registry build #20622

kariya-mitsuru · 2024-06-19T07:02:17Z

Comprehensive Summary of your change

make/photon/Makefile

Remove cd command since the current directory is changed in make/photon/registry/builder.
make/photon/registry/Dockerfile.base

Change the owner of /etc/pki/tls/certs when building the base image.
(Since files under /etc/pki/tls/certs are not affected by binary build.)
make/photon/registry/Dockerfile
- Remove changing the owner of /etc/pki/tls/certs since the change has been moved to the base image.
- The owner/permission changes of files copied from the context are now performed simultaneously when the COPY command is executed.
  (If COPY and the owner/permission changes were separated, both image layers before and after change would be created, making the image unnecessarily large.)
- Add --link option to COPY command.
  (This will improve image build efficiency since the base image will not be extracted at build time.)
make/photon/registry/builder.
- Move set -e (exit immediately on error) to the top.
- There is no error command, so change it to the echo command.
- Remove cur variables that are no longer used by using ~- and cd -.
- Add --depth 1 option to git clone command.
  (Since we only need the specified version of the source to build, we don't need the whole history, and this reduces the amount of transfer at clone time.)
- Change the docker build command to specify the source file directly with the -f option instead of copying Dockerfile.binary.
- Change the docker build command to directly output the binary file without creating a container image and container by specifying the output directory with the -o option.
make/photon/registry/Dockerfile.binary.
- Remove PREFIX variable specified at make command line, since it is not used.
- Change make build target from binaries to bin/registry.
  (Since binaries other than bin/registry are not used.)
- Add a stage to extract only binary files, since make/photon/registry/builder now outputs binary files directly.

Issue being fixed

Please indicate you've done the following:

Well Written Title and Summary of the PR
Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
Accepted the DCO. Commits without the DCO will delay acceptance.
Made sure tests are passing and test coverage is added if needed.
Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

1. `make/photon/Makefile` Remove `cd` command since the current directory is changed in `make/photon/registry/builder`. 2. `make/photon/registry/Dockerfile.base` Change the owner of `/etc/pki/tls/certs` when building the base image. (Since files under `/etc/pki/tls/certs` are not affected by binary build.) 3. `make/photon/registry/Dockerfile` - Remove changing the owner of `/etc/pki/tls/certs` since the change has been moved to the base image. - The owner/permission changes of files copied from the context are now performed simultaneously when the `COPY` command is executed. (If `COPY` and the owner/permission changes were separated, both image layers before and after change would be created, making the image unnecessarily large.) - Add `--link` option to `COPY` command. (This will improve image build efficiency since the base image will not be extracted at build time.) 4. `make/photon/registry/builder`. - Move `set -e` (exit immediately on error) to the top. - There is no `error` command, so change it to the `echo` command. - Remove `cur` variables that are no longer used by using `~-` and `cd -`. - Add `--depth 1` option to `git clone` command. (Since we only need the specified version of the source to build, we don't need the whole history, and this reduces the amount of transfer at clone time.) - Change the `docker build` command to specify the source file directly with the `-f` option instead of copying `Dockerfile.binary`. - Change the `docker build` command to directly output the binary file without creating a container image and container by specifying the output directory with the `-o` option. 5. `make/photon/registry/Dockerfile.binary`. - Remove `PREFIX` variable specified at `make` command line, since it is not used. - Change `make` build target from `binaries` to `bin/registry`. (Since `binaries` other than `bin/registry` are not used.) - Add a stage to extract only binary files, since `make/photon/registry/builder` now outputs binary files directly. Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

codecov · 2024-07-09T13:45:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.73%. Comparing base (c8c11b4) to head (e611f70).
Report is 277 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #20622       +/-   ##
===========================================
+ Coverage   45.36%   66.73%   +21.36%     
===========================================
  Files         244     1047      +803     
  Lines       13333   114271   +100938     
  Branches     2719     2847      +128     
===========================================
+ Hits         6049    76259    +70210     
- Misses       6983    34132    +27149     
- Partials      301     3880     +3579

Flag	Coverage Δ
unittests	`66.73% <ø> (+21.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1287 files with indirect coverage changes

make/photon/registry/Dockerfile.base

make/photon/registry/Dockerfile

wy65701436 · 2024-08-01T06:59:12Z

make/photon/registry/builder


 # the temp folder to store distribution source code...
 TEMP=`mktemp -d ${TMPDIR-/tmp}/distribution.XXXXXX`
-git clone -b $VERSION https://github.com/distribution/distribution.git $TEMP
+git clone -b $VERSION --depth 1 https://github.com/distribution/distribution.git $TEMP


why is it necessary to specify the depth here?

If --depth 1 is not specified, then the entire history of all the commits to the $VERSION will be cloned, which is unnecessary and is wasteful in terms of transfer amount and processing time (and very painful for a poor environment like mine).
By specifying --depth 1, only the commits specified by $VERSION can be retrieved.

wy65701436 · 2024-08-01T07:06:27Z

make/photon/registry/builder


 # add patch redis
 cd $TEMP
-git apply $cur/redis.patch
-cd $cur
+git apply ~-/redis.patch


does it support all the linux OS versions?

Yes.
It is supported by all currently used bash.

wy65701436 · 2024-08-01T07:10:07Z

make/photon/registry/Dockerfile.binary

+RUN CGO_ENABLED=0 make clean bin/registry
+
+FROM scratch
+COPY --from=build /go/src/github.com/docker/distribution/bin/registry /


to /bin/binary?

It was originally created in binary, not bin/binary, does that mean it should be changed to bin/binary?

wy65701436 · 2024-08-01T07:11:35Z

make/photon/registry/Dockerfile.binary

@@ -7,4 +7,7 @@ ENV GO111MODULE auto
 WORKDIR $DISTRIBUTION_DIR
 COPY . $DISTRIBUTION_DIR

-RUN CGO_ENABLED=0 make PREFIX=/go clean binaries
+RUN CGO_ENABLED=0 make clean bin/registry
+


why is this change needed?

The PREFIX variable is not used anywhere, which leads to confusion.
Therefore, I think it is better to remove it.

wy65701436 · 2024-08-01T07:13:19Z

make/photon/Makefile

@@ -178,7 +178,7 @@ _build_registry:
 		rm -rf $(DOCKERFILEPATH_REG)/binary && mkdir -p $(DOCKERFILEPATH_REG)/binary && \
 		$(call _get_binary, $(REGISTRYURL), $(DOCKERFILEPATH_REG)/binary/registry); \
 	else \
-		cd $(DOCKERFILEPATH_REG) && $(DOCKERFILEPATH_REG)/builder $(REGISTRY_SRC_TAG) && cd - ; \
+		$(DOCKERFILEPATH_REG)/builder $(REGISTRY_SRC_TAG) ; \


does this mean the builder script cannot be executed in the location of $(DOCKERFILEPATH_REG)?

No.
The builder script can be executed in the location of $(DOCKERFILEPATH_REG), but it also can be executed in any other location, so it need not to cd to $(DOCKERFILEPATH_REG).

Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

Modify `FROM as` to `FROM AS` to suppress Docker warnings Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

chlins

lgtm

chlins · 2024-09-20T03:44:06Z

Take note that this modification relies on certain new features of Docker Buildkit. As a result, you might encounter an error when building with an older version of Docker.

Vad1mo · 2024-09-20T08:16:22Z

Take note that this modification relies on certain new features of Docker Buildkit. As a result, you might encounter an error when building with an older version of Docker.

Do you have an idea how new the docker version should be?

I scanned the Dockerfile, and based on the output it should run on Docker 17.06 CE (June 2017)

We should be pritty save on that side

chlins · 2024-09-20T08:58:25Z

AFAIK, RUN --mount was introduced since https://docs.docker.com/build/buildkit/dockerfile-release-notes/#130, and the corresponding docker version should be v19.03.

kariya-mitsuru requested a review from a team as a code owner June 19, 2024 07:02

github-actions bot assigned AllForNothing, OrlinVasilev and zyyw Jun 19, 2024

kariya-mitsuru mentioned this pull request Jun 20, 2024

Reduce image size #20640

Open

5 tasks

MinerYang assigned wy65701436 and unassigned zyyw, OrlinVasilev and AllForNothing Jun 24, 2024

Vad1mo approved these changes Jul 9, 2024

View reviewed changes

wy65701436 reviewed Jul 31, 2024

View reviewed changes

make/photon/registry/Dockerfile.base Outdated Show resolved Hide resolved

wy65701436 reviewed Jul 31, 2024

View reviewed changes

make/photon/registry/Dockerfile Outdated Show resolved Hide resolved

wy65701436 reviewed Aug 1, 2024

View reviewed changes

kariya-mitsuru and others added 5 commits August 8, 2024 01:02

Revert make/photon/registry/Dockerfile.base

c0c2265

Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

Use RUN with bind mount instead of COPY

584df97

Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

Capitalize FROM AS

28a35ba

Modify `FROM as` to `FROM AS` to suppress Docker warnings Signed-off-by: Mitsuru Kariya <mitsuru.kariya@nttdata.com>

Merge branch 'main' into simplify-registry-build

8578deb

Merge branch 'main' into simplify-registry-build

e611f70

chlins added release-note/update Update or Fix release-note/infra Infra related changes e.g. release, test, ship etc... and removed release-note/update Update or Fix labels Sep 20, 2024

chlins approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify registry build #20622

Simplify registry build #20622

kariya-mitsuru commented Jun 19, 2024

codecov bot commented Jul 9, 2024 •

edited

Loading

wy65701436 Aug 1, 2024

kariya-mitsuru Aug 1, 2024

wy65701436 Aug 1, 2024

kariya-mitsuru Aug 1, 2024

wy65701436 Aug 1, 2024

kariya-mitsuru Aug 1, 2024

wy65701436 Aug 1, 2024

kariya-mitsuru Aug 1, 2024

wy65701436 Aug 1, 2024

kariya-mitsuru Aug 1, 2024

chlins left a comment

chlins commented Sep 20, 2024

Vad1mo commented Sep 20, 2024

chlins commented Sep 20, 2024

Simplify registry build #20622

Are you sure you want to change the base?

Simplify registry build #20622

Conversation

kariya-mitsuru commented Jun 19, 2024

Comprehensive Summary of your change

Issue being fixed

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlins left a comment

Choose a reason for hiding this comment

chlins commented Sep 20, 2024

Vad1mo commented Sep 20, 2024

chlins commented Sep 20, 2024

codecov bot commented Jul 9, 2024 •

edited

Loading